Skip to content

Consider config.relative_url_root in ActsAsResourceController.base_url#1282

Merged
lgebhardt merged 3 commits intoJSONAPI-Resources:masterfrom
tobias-bischoff:patch-2
Sep 16, 2019
Merged

Consider config.relative_url_root in ActsAsResourceController.base_url#1282
lgebhardt merged 3 commits intoJSONAPI-Resources:masterfrom
tobias-bischoff:patch-2

Conversation

@tobias-bischoff
Copy link
Copy Markdown
Contributor

When Rails is deployed to a subdirectory per the official guide instructions, JSON:API should take this into account.

Implementation detail: Switched from string concatenation (+) to string interpolation because Rails.application.config.relative_url_root might be nil, which would cause an error as @hlogmans already stated.

Tested with:

  • Setting RAILS_RELATIVE_URL_ROOT=/subdirectory
  • Adding config.relative_url_root = '/subdirectory' in application.rb or .rb

Fixes #473

All Submissions:

  • I've checked to ensure there aren't other open Pull Requests for the same update/change.
  • I've submitted a ticket for my issue if one did not already exist.
  • My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
  • I've used Github auto-closing keywords in the commit message or the description.
  • I've added/updated tests for this change.

Bug fixes and Changes to Core Features:

  • I've included an explanation of what the changes do and why I'd like you to include them.
  • I've provided test(s) that fails without the change.

Test Plan:

Unit test: bundle exec ruby -I test test/controllers/controller_test.rb -n test_links_include_relative_root

Manual testing:

  1. Add config.relative_url_root = '/subdirectory' to config/application.rb
  2. Run Rails server
  3. Fetch any resource URL
    Expected result: Link objects contain /subdirectory

Reviewer Checklist:

  • Maintains compliance with JSON:API
  • Adequate test coverage exists to prevent regressions

@tobias-bischoff tobias-bischoff changed the title Patch 2 Consider config.relative_url_root in ActsAsResourceController.base_url Sep 4, 2019
@tobias-bischoff tobias-bischoff changed the title Consider config.relative_url_root in ActsAsResourceController.base_url Consider config.relative_url_root in ActsAsResourceController.base_url Sep 4, 2019
@tobias-bischoff
Copy link
Copy Markdown
Contributor Author

tobias-bischoff commented Sep 4, 2019

Note: I ran the test suite locally (ruby 2.6.3, rails 5.2.3, command bundle exec rake test) without cerebris/jsonapi-resources@c6f11c3 and it passed just fine:

Finished in 120.243376s, 5.5055 runs/s, 71.4052 assertions/s.
662 runs, 8586 assertions, 0 failures, 0 errors, 0 skips

However, Travis CI jobs all failed because subsequent tests suddenly included /subdir in their link responses. So I reset Rails.application.config.relative_url_root after the assertion, hope that provides enough isolation. Still wondering why the spillover only happens on Travis.

@tobias-bischoff
Copy link
Copy Markdown
Contributor Author

@lgebhardt FYI: It seems Travis fails on specific Ruby/Rails combinations due to a bundler version error. This seems unrelated to my PR since it's also happening in other PRs. The builds where bundler actually runs are fine.

tobias-bischoff and others added 3 commits September 16, 2019 13:54
When Rails is deployed to a subdirectory per [the official guide instructions](https://guides.rubyonrails.org/configuring.html#deploy-to-a-subdirectory-relative-url-root), JSON:API should take this into account.

Implementation detail: Switched from string concatenation (`+`) to string interpolation because `Rails.application.config.relative_url_root` might be `nil`, which would cause an error as @hlogmans [already stated](https://github.com/cerebris/jsonapi-resources/issues/473#issuecomment-153719383).

Tested with:
- Setting `RAILS_RELATIVE_URL_ROOT=/subdirectory`
- Adding `config.relative_url_root = '/subdirectory'` in application.rb or <environment>.rb

Fixes #473
Signed-off-by: Tobias Grasse <tg@glancr.de>
Test suite runs fine without this locally (ruby 2.6.3, Rails 5.2.3), but Travis CI builds all fail because subsequent tests expect a clean base URL.

Signed-off-by: Tobias Grasse <tg@glancr.de>
@lgebhardt lgebhardt merged commit 81a51ca into JSONAPI-Resources:master Sep 16, 2019
@lgebhardt
Copy link
Copy Markdown
Contributor

@tobias-grasse Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make acts_as_resource_controller's base_url method be compatible with custom Rails config.relative_url_root

2 participants